-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
ENH: add to_offset method to Timedelta #9064 #9226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Another option is to ensure it is first a Some other things:
|
your method will not handle < 1 sec if its not divisible. better to start go thru the time hierarchy and add offsets. e.g. |
@jorisvandenbossche working on these
@jreback my initial understanding was that converting everything into total_seconds would serve the purpose. However, going through the heirarchy and adding the offsets certainly sounds like a better way. Looking into this. Thanks, |
@jorisvandenbossche I will add more tests. |
A question. @jreback when you say .components, are you referring to something similar to what I found in timedeltas.rst? -> "You can use the .components property to access a reduced form of the timedelta. This returns a DataFrame indexed similarly to the Series" or did you mean directly use the attributes days,hours,minutes,seconds,milliseconds,microseconds,nanoseconds? Thanks. |
.components of a TimeDelta is a named tuple with the data u need |
you can better use the |
Ok thanks. I am seeing an inconsistency though. pd.Timedelta doesn't allow nanoseconds in constructor but its components list till nanoseconds. Here is what I can reproduce. Python 2.7.9 (v2.7.9:648dcafa7e5f, Dec 10 2014, 10:10:46) [GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> from pandas.tslib import Timedelta >>> td = Timedelta(nanoseconds=1) Traceback (most recent call last): File "", line 1, in File "pandas/tslib.pyx", line 1723, in pandas.tslib.Timedelta.__new__ (pandas/tslib.c:29743) raise ValueError("cannot construct a TimeDelta from the passed arguments, allowed keywords are " [days, seconds, microseconds, milliseconds, minutes, hours, weeks] >>> td=Timedelta(seconds=1) >>> td.components._fields ('days', 'hours', 'minutes', 'seconds', 'milliseconds', 'microseconds', 'nanoseconds') |
hmm u can add that if u want (and a separate test) |
Also, if I try to add Nano() DateOffset to any other pd.DateOffset on my machine, I get this error: TypeError: ufunc add cannot use operands with types dtype('O') and dtype('<m8[ns]'). Something I need to fix in my build etc.? Example: >>> from pandas import offsets >>> d1=offsets.Second(1) >>> d2=offsets.Milli(1) >>> d3=offsets.Micro(1) >>> d4=offsets.Nano(1) >>> 5*d1+4*d2+3*d3+2*d4 Traceback (most recent call last): File "", line 1, in File "pandas/tseries/offsets.py", line 2027, in __add__ return _delta_to_tick(self.delta + other.delta) TypeError: ufunc add cannot use operands with types dtype('O') and dtype('<m8[ns]') >>> 5*d1+4*d2+3*d3 <5004003 * Micros> >>> 5*d1+4*d2+0*d3 <5004 * Millis> >>> 5*d1+4*d2+1*d3 <5004001 * Micros> Thanks, |
Thanks @jreback I will club adding nanoseconds in constructor to this change. |
hmm adding a Nano should work |
Ok thanks, I will need to look into these more and see if I spot a fix. I guess these need to be fixed first to properly traverse hierarchy and add offsets otherwise things seem to be working fine till microseconds. |
@jorisvandenbossche, @jreback I have updated the PR with the following changes:
PLMK if there are better ways to get DateOffset objects from td.components._fields. I have created a local name to offset object map in the method. Will attend to review comments as they come by. In the meantime, I will look into: a) why adding Nano to DateOffset is failing and b) adding support of Nano in pd.Timedelta constructor. |
@@ -278,7 +278,8 @@ def get_period_alias(offset_str): | |||
|
|||
def to_offset(freqstr): | |||
""" | |||
Return DateOffset object from string representation | |||
Return DateOffset object from string representation or | |||
datetime.timedelta or pandas.tslib.Timedelta objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pandas.tslib.Timedelta -> pandas.Timedelta (that is how it is exposed to the users)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
From what I see, support for nano is broken in Timedelta and I am able to reproduce that in various ways. However, it seems like this ENH should should not need any further changes related to the nano fixes. Would it be a good idea to treat these two separately? I can go ahead and create a separate issue for the nano fix and continue the fix related discussions there, while this ENH can be merged independently. |
@@ -298,6 +304,28 @@ def to_offset(freqstr): | |||
name, stride = stride, name | |||
name, _ = _base_and_stride(name) | |||
delta = get_offset(name) * stride | |||
|
|||
elif isinstance(freqstr, tslib.Timedelta): | |||
name_to_offset_map = {'days': Day(1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put this as a module level variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good. done.
couple of minor changes, pls rebase and squash. |
delta = None | ||
|
||
if isinstance(freqstr, timedelta): | ||
freqstr = tslib.Timedelta(freqstr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test is not needed, you can just do isinstance(freqstr, timedelta)
below, and then first convert to Timedelta
(pandas.Timedelta will also be an instance of timedelta)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. done
Incorporated review comments and squashed. Created a new issue to track nano fix here. #9273. Thanks. |
ok, looks good. Pls add a release note referencing the original issue. You can put it in the enhancements section. |
@jreback done adding note |
@@ -99,6 +99,7 @@ Enhancements | |||
- Added time interval selection in get_data_yahoo (:issue:`9071`) | |||
- Added ``Series.str.slice_replace()``, which previously raised NotImplementedError (:issue:`8888`) | |||
- Added ``Timestamp.to_datetime64()`` to complement ``Timedelta.to_timedelta64()`` (:issue:`9255`) | |||
- ``pandas.tseries.frequencies.to_offset()`` now accepts ``pandas.Timdelta`` or ``datetime.timedelta`` as input (:issue:`9064`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just say Timedelta
(spelling too)
minor import correction, otherwise looks good. ping after you push and its green. |
to_offset now accepts Timedelta as input
Ok, changed imports, docstring and release note. Thanks. |
ENH: add to_offset method to Timedelta #9064
@tvyomkesh thanks! |
Closes #9064